Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[occ] Fix parent store readset validation #348

Merged
merged 4 commits into from
Nov 10, 2023
Merged

Conversation

udpatil
Copy link
Contributor

@udpatil udpatil commented Nov 10, 2023

Describe your changes and provide context

This fixes the validation to remove a panic for a case that can actually occur if a transaction writes a key that is later read, and that writing transaction is reverted and then the readset validation reads from parent store. In this case, the readset would have a conflict based on the data available in parent store, so we shouldn't panic. This also adds in the resource types needed for the new DEX_MEM keys

Testing performed to validate your change

Tested in loadtest cluster

@udpatil udpatil changed the title Fix parent validation [occ] Fix parent store readset validation Nov 10, 2023
Copy link
Contributor

@stevenlanders stevenlanders left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm (I would just suggest a unit test for the scenario if possible)

@udpatil
Copy link
Contributor Author

udpatil commented Nov 10, 2023

lgtm (I would just suggest a unit test for the scenario if possible)

yup good point, will add

Copy link

codecov bot commented Nov 10, 2023

Codecov Report

Merging #348 (490d80b) into occ-main (931e2f6) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           occ-main     #348   +/-   ##
=========================================
  Coverage     56.12%   56.12%           
=========================================
  Files           627      627           
  Lines         52859    52860    +1     
=========================================
+ Hits          29668    29669    +1     
  Misses        21084    21084           
  Partials       2107     2107           
Files Coverage Δ
store/multiversion/store.go 95.18% <100.00%> (+0.01%) ⬆️
types/accesscontrol/resource.go 0.00% <ø> (ø)

@udpatil udpatil merged commit eac8657 into occ-main Nov 10, 2023
15 checks passed
@udpatil udpatil deleted the fix-parent-validation branch November 10, 2023 21:43
udpatil added a commit that referenced this pull request Jan 2, 2024
## Describe your changes and provide context
This fixes the validation to remove a panic for a case that can actually
occur if a transaction writes a key that is later read, and that writing
transaction is reverted and then the readset validation reads from
parent store. In this case, the readset would have a conflict based on
the data available in parent store, so we shouldn't panic. This also
adds in the resource types needed for the new DEX_MEM keys

## Testing performed to validate your change
Tested in loadtest cluster
udpatil added a commit that referenced this pull request Jan 8, 2024
This fixes the validation to remove a panic for a case that can actually
occur if a transaction writes a key that is later read, and that writing
transaction is reverted and then the readset validation reads from
parent store. In this case, the readset would have a conflict based on
the data available in parent store, so we shouldn't panic. This also
adds in the resource types needed for the new DEX_MEM keys

Tested in loadtest cluster
udpatil added a commit that referenced this pull request Jan 8, 2024
This fixes the validation to remove a panic for a case that can actually
occur if a transaction writes a key that is later read, and that writing
transaction is reverted and then the readset validation reads from
parent store. In this case, the readset would have a conflict based on
the data available in parent store, so we shouldn't panic. This also
adds in the resource types needed for the new DEX_MEM keys

Tested in loadtest cluster
udpatil added a commit that referenced this pull request Jan 18, 2024
This fixes the validation to remove a panic for a case that can actually
occur if a transaction writes a key that is later read, and that writing
transaction is reverted and then the readset validation reads from
parent store. In this case, the readset would have a conflict based on
the data available in parent store, so we shouldn't panic. This also
adds in the resource types needed for the new DEX_MEM keys

Tested in loadtest cluster
udpatil added a commit that referenced this pull request Jan 18, 2024
## Describe your changes and provide context
This fixes the validation to remove a panic for a case that can actually
occur if a transaction writes a key that is later read, and that writing
transaction is reverted and then the readset validation reads from
parent store. In this case, the readset would have a conflict based on
the data available in parent store, so we shouldn't panic. This also
adds in the resource types needed for the new DEX_MEM keys

## Testing performed to validate your change
Tested in loadtest cluster
udpatil added a commit that referenced this pull request Jan 25, 2024
## Describe your changes and provide context
This fixes the validation to remove a panic for a case that can actually
occur if a transaction writes a key that is later read, and that writing
transaction is reverted and then the readset validation reads from
parent store. In this case, the readset would have a conflict based on
the data available in parent store, so we shouldn't panic. This also
adds in the resource types needed for the new DEX_MEM keys

## Testing performed to validate your change
Tested in loadtest cluster
udpatil added a commit that referenced this pull request Jan 31, 2024
## Describe your changes and provide context
This fixes the validation to remove a panic for a case that can actually
occur if a transaction writes a key that is later read, and that writing
transaction is reverted and then the readset validation reads from
parent store. In this case, the readset would have a conflict based on
the data available in parent store, so we shouldn't panic. This also
adds in the resource types needed for the new DEX_MEM keys

## Testing performed to validate your change
Tested in loadtest cluster
codchen pushed a commit that referenced this pull request Feb 6, 2024
This fixes the validation to remove a panic for a case that can actually
occur if a transaction writes a key that is later read, and that writing
transaction is reverted and then the readset validation reads from
parent store. In this case, the readset would have a conflict based on
the data available in parent store, so we shouldn't panic. This also
adds in the resource types needed for the new DEX_MEM keys

Tested in loadtest cluster
udpatil added a commit that referenced this pull request Feb 27, 2024
## Describe your changes and provide context
This fixes the validation to remove a panic for a case that can actually
occur if a transaction writes a key that is later read, and that writing
transaction is reverted and then the readset validation reads from
parent store. In this case, the readset would have a conflict based on
the data available in parent store, so we shouldn't panic. This also
adds in the resource types needed for the new DEX_MEM keys

## Testing performed to validate your change
Tested in loadtest cluster
udpatil added a commit that referenced this pull request Mar 1, 2024
## Describe your changes and provide context
This fixes the validation to remove a panic for a case that can actually
occur if a transaction writes a key that is later read, and that writing
transaction is reverted and then the readset validation reads from
parent store. In this case, the readset would have a conflict based on
the data available in parent store, so we shouldn't panic. This also
adds in the resource types needed for the new DEX_MEM keys

## Testing performed to validate your change
Tested in loadtest cluster
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants